Fix some titles and get the full sea ice diagnostics working again #162
Conversation
|
Hi @dabail10 is this ready for review? |
|
This should be ready for review. I realize the coupled_model/config.yml file is still using the "old" variables. Perhaps I should update this. |
There was a problem hiding this comment.
I think you need to modify lines 150 and 160 in the config.yml file -- 150 still uses seaice as the name of the notebook in the compute_notebooks.ice section, and 160 uses nyears instead of climo_nyears when setting parameters for the notebook. (I haven't tried running this yet, but just from looking would expect these to fail.)
|
also, the |
|
Whoops. I had tested with a custom version of config.yml (config_seaice.yml). The compare_obs_lens is run in the key_metrics, while the full is only run in the coupled_model area. |
|
After talking with @mnlevy1981 I have reworked this PR. Have a look at tell me what you think. |
There was a problem hiding this comment.
These notebooks ran for both coupled_model (both notebooks) and key_metrics (just the one notebook). FYI the full notebook had some plots with overlapping titles, but I don't think that's a significant concern.
Also note that there are a few other non-related failures in coupled-model (a register_cmap issue in ocean_surface.ipynb, as well as an adf_diag import error. This should be noted in a separate issue (which I will make).
I did just remove the unused imports for pandas and matplotlib.pyplot-- they cause DAG warnings and may confuse users.
All that said, this looks good to me.
|
Issue ticket #140 corresponds to comment above |
|
I have a couple more fixes for the timeseries part. |
|
Ok, great. Also can you please clear the outputs from your notebooks before committing them? |
|
Whoops. Working on it. |
| "hide-input" | ||
| ] | ||
| }, | ||
| "outputs": [ |
There was a problem hiding this comment.
This one still has outputs. The other one looks good.
|
I think that has everything. |
|
Looks much better! This is all good by me! I'll let @mnlevy1981 take one more look since you all discussed together and I want to make sure he's on board with these changes. Otherwise I think this is good to go! |
Also, we want to print(client) for both CUPiD and non-CUPiD runs
Description of changes:
I have changed the "full" notebook to be just the contour plots. Then coupled_model/config.yml points
to both the _obs_lens and _contour notebooks. I have updated the coupled_model config file to also use the case_name / base_case_name syntax.
pre-commitchecks passed (#8 in Adding Notebooks Guide)?